Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC #218: Python vendoring #218

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gsnedders
Copy link
Member

@gsnedders gsnedders commented Jan 7, 2025

@Ms2ger suggested I file an RFC for this.

Rendered.

Implementation: web-platform-tests/wpt#49752.

@gsnedders gsnedders changed the title Add an RFC about vendoring our Python RFC #218: Python vendoring Jan 7, 2025
@jcscottiii
Copy link
Contributor

@WeizhongX I don't think this will impact Chromium CI but I thought I would double check with you.

@WeizhongX
Copy link
Contributor

@WeizhongX I don't think this will impact Chromium CI but I thought I would double check with you.

We might need to create vpython wheel for the vendor packages, but should be fine for us.

@jgraham
Copy link
Contributor

jgraham commented Jan 8, 2025

This generally seems fine to me, and looks like a significant improvement over the status quo. I have the following comments:

  • I think the risk of this affecting Mozilla's prioritization of improving our story around non-vendored deps is minimal. Most of the pressure there really comes from deps with binary components, which aren't changed either way by this proposal.
  • I think there is some risk that vendoring changes in the future in a way that breaks our use. It's specifically documented as being unstable and only designed to work with pip. I think it's a managable risk, but a real one.
  • There is also the risk that this breaks unshared tests using wpt, if they are doing custom path manipulations. That isn't any kind of blocker, but people should be aware, since it may affect syncing.
  • The implementation should be tested with full runs of wpt using the trigger branches, not just relying on CI jobs, since there's a risk of any test that does something custom with the Python path breaking.

rfcs/vendoring.md Outdated Show resolved Hide resolved
@gsnedders
Copy link
Member Author

  • I think there is some risk that vendoring changes in the future in a way that breaks our use. It's specifically documented as being unstable and only designed to work with pip. I think it's a managable risk, but a real one.

This is fair, but it's essentially "we now use a tool to automate most of the work that may break in future" (and really it isn't a complex tool) versus "we don't really document how to vendor new things and don't really give people guidance on how to do it".

Like, the ultimate fallback is just directly doing what vendoring does:

    command = [
        "pip",
        "install",
        "--platform",
        "any",
        "-t",
        str(destination),
        "-r",
        str(requirements),
        "--no-compile",
        # We use --no-deps because we want to ensure that dependencies are provided.
        # This includes all dependencies recursively up the chain.
        "--no-deps",
    ]

Though this means we then miss things like "check we get all the licenses", and automatically applying patches, and potentially rewriting module names.

  • The implementation should be tested with full runs of wpt using the trigger branches, not just relying on CI jobs, since there's a risk of any test that does something custom with the Python path breaking.

Per web-platform-tests/wpt#49752 (comment) this all looks like pre-existing flakiness to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants